Replace doi2bib with a util function to remove future package dependency #770
Conversation
|
The boolean return value is never actually used anywhere this is called, so overall it probably makes since to change the API of this function. However the bibtex library seems to be going from v1 to v2 and a lot of the view code is going to have to be refactored anyways as some point. So I argue that it should all just be left for another day. This is fine for now. |
|
@jrlagrone thanks for catching this! |
|
@jrlagrone Make sure to squash and sign the commit when you are ready for his pr to be merged. |
jrlagrone
force-pushed
the
remove_doi2bib
branch
from
fc4c115 to
f083a85
Compare
Signed-off-by: John LaGrone <[email protected]>
jrlagrone
force-pushed
the
remove_doi2bib
branch
from
f083a85 to
6dc02a5
Compare
|
@Eric-Butcher that should be a single signed commit now. It did change the uv file more than I expected, but it looks like that's mostly But yes on both fronts -- I think being able to remove 2 dependencies with ~10 lines of code is good, but also that it should probably be refactored (more resilient, staying up to date with the bibtex libs as you mentioned, actually following the best practices from crossref https://api.crossref.org/swagger-ui/index.html#best-practice, etc.) |
ANekhai
left a comment
ANekhai
left a comment
There was a problem hiding this comment.
Nice job removing that stale dependency with a simple util function, looks good to merge to me.
aebruno
left a comment
aebruno
left a comment
There was a problem hiding this comment.
This is mainly to address CVE-2025-50817
This CVE has been withdrawn and determined not to be a security flaw in python-future.
doi2bib appears to no longer be actively maintained.
What if we switch to another module that is actively maintained? I suggest we either look at crossrefapi or habanero. Both of these appear actively maintained with good APIs. This way we can likely simplify this one step further and get rid of the BibTexParser code as well.
It did change the uv file more than I expected, but it looks like that's mostly upload-time fields so I assume that's not really an issue. I'm pretty new to UV though, so it might be good to look at that closely.
The changes to the uv.lock file are likely due to using a newer version of uv which added upload-time to the lock file. This also bumps the lock file version. I'll be making these changes in a separate PR along with adding a tool.uv.required-version.
In summary, here's some initial feedback on this PR:
- Let's modify the
def get_bib(doi_number):function to use one of the modules suggested above. The function should just return the parsed python dict with all the publication data parsed or raise an exception on error. This way callers will have all the data they need without having to parse bibtex. - This may require some more code changes to the bits that use the parsed data structure as it might be slightly different but shouldn't be too bad as all the same data is there.
- Remove the uv.lock changes as those will be included in another PR.
|
I'm good with that. It might be a while before I can work on it -- I can try to prioritize it if you want to merge it before migrating repos. Otherwise, I think I'm good closing or changing this to WIP for now and including it as a recommended improvement / refactor for "later". Currently nothing is broken. |
4 participants
Replace the functionality of the
git_bibfunction fromdoi2bib.This is mainly to address CVE-2025-50817 --
doi2bibuses thefuturepackage anddoi2bibappears to no longer be actively maintained. It appears to me that the functionality being used in ColdFront is a simple url request to the crossref.org rest api so just re-implement that.Note: the following have not been updated (but should be).
the tests in publication will no longer work as written (I didn't fully comprehend what it was actually doing, but I can circle back when I have more time available and try to update them)The packagesdoi2bibandfutureshould no longer be dependencies in theuvfile. I'm still running an older version usingsetup.py/requirements.txt, so I didn't test. My assumption is that is a trivial change.